Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make u32 shift and rotation instructions always checked #1135

Closed
wants to merge 2 commits into from

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Nov 2, 2023

This small PR changes a little bit u32shl, u32shr, u32rotl and u32totr instructions to make them always checked — input values are always validated to be a u32 and a value less than 32.

@Fumuran Fumuran requested a review from bobbinth November 2, 2023 17:47
@Fumuran Fumuran linked an issue Nov 2, 2023 that may be closed by this pull request
Comment on lines 53 to 56
| u32shl <br> - *(47 cycles)* <br> u32shl.*b* <br> - *(4 cycles)* | [b, a, ...] | [c, ...] | $c \leftarrow (a \cdot 2^b) \mod 2^{32}$ <br> Fails if $a \ge 2^{32}$ or $b > 31$ |
| u32shr <br> - *(47 cycles)*<br> u32shr.*b* <br> - *(4 cycles)* | [b, a, ...] | [c, ...] | $c \leftarrow \lfloor a/2^b \rfloor$ <br> Fails if $a \ge 2^{32}$ or $b > 31$ |
| u32rotl <br> - *(47 cycles)* <br> u32rotl.*b* <br> - *(4 cycles)* | [b, a, ...] | [c, ...] | Computes $c$ by rotating a 32-bit representation of $a$ to the left by $b$ bits. <br> Fails if $a \ge 2^{32}$ or $b > 31$ |
| u32rotr <br> - *(59 cycles)* <br> u32rotr.*b* <br> - *(6 cycles)* | [b, a, ...] | [c, ...] | Computes $c$ by rotating a 32-bit representation of $a$ to the right by $b$ bits. <br> Fails if $a \ge 2^{32}$ or $b > 31$ |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were the previous counts incorrect? Or did this really increase cycle counts for non-immediate operations by 7 - 15 cycles?

Also, would be curious to know how these changes affected things like SHA256 and BLAKE3 computations (we have these in stdlib - maybe would be a good idea to put together simple examples with them, and then see before/after cycle counts).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, these values are not correct. I'm not even sure why were these values like this before. I've updated the functions comments and documentation.

@Fumuran
Copy link
Contributor Author

Fumuran commented Nov 3, 2023

I tested hash_2to1 and hash_1to1 functions from blake3 and sha256 modules with old implementation (shift ant rotate are always unchecked) and new implementation (always checked).
Here are the results:

Screenshot 2023-11-03 at 8 09 05 PM

@Fumuran Fumuran requested a review from bobbinth November 3, 2023 19:13
@bobbinth
Copy link
Contributor

bobbinth commented Nov 7, 2023

Based on yesterday's discussion, I think we should probably leave the implementation as is (i.e., keep the unchecked implementation). A couple of things we could still do:

  • Update the docs to reflect the correct cycle counts.
  • Maybe introduce assert_lt instruction (so that it would be possible to do something like assert_lt.32) - though, this should probably go into a different PR (and we should probably create a separate issue for this).

@bobbinth
Copy link
Contributor

bobbinth commented Nov 8, 2023

btw, if you have the code for running blake3 and sha256, would be great to add it to the examples.

@Fumuran Fumuran closed this Nov 9, 2023
@Fumuran Fumuran removed the request for review from bobbinth November 9, 2023 13:19
@Fumuran Fumuran deleted the andrew-make-sh-rot-checked branch November 9, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make u32sh* and u32rot* instructions always checked
2 participants